Durga js events#539
Conversation
Summary of ChangesHello @Saidurgasatturi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a series of new assignments and examples primarily focused on fundamental JavaScript event handling and DOM manipulation techniques. It covers essential concepts such as event flow, preventing default browser actions, and dynamic content updates, alongside some foundational CSS styling and animation demonstrations. The changes aim to provide clear, practical illustrations of these web development principles. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds several assignments demonstrating CSS features and JavaScript event handling. The implementations are a good start, but there are several areas for improvement regarding correctness, performance, and adherence to best practices. Key issues identified include inefficient event handlers causing flickering, fragile form validation logic that can fail with autofill, and usability bugs in interactive components. I've provided specific comments and suggestions to address these points and improve the overall quality of the code.
| document.getElementById("myForm").addEventListener("submit", function(event) { | ||
| const name = document.getElementById("name").value.trim(); | ||
| const email = document.getElementById("email").value.trim(); | ||
| const message = document.getElementById("message"); | ||
| if (name === "" || email === "") { | ||
| event.preventDefault(); | ||
| message.textContent = "Please fill out all fields before submitting!"; | ||
| } else { | ||
| message.style.color = "green"; | ||
| message.textContent = "Form submitted successfully!"; | ||
| } | ||
| }); |
There was a problem hiding this comment.
The form submission is not prevented on success, which causes the page to reload immediately. As a result, the user will never see the 'Form submitted successfully!' message. A better pattern is to call event.preventDefault() at the beginning of the handler and then proceed with validation and displaying the appropriate message.
| document.getElementById("myForm").addEventListener("submit", function(event) { | |
| const name = document.getElementById("name").value.trim(); | |
| const email = document.getElementById("email").value.trim(); | |
| const message = document.getElementById("message"); | |
| if (name === "" || email === "") { | |
| event.preventDefault(); | |
| message.textContent = "Please fill out all fields before submitting!"; | |
| } else { | |
| message.style.color = "green"; | |
| message.textContent = "Form submitted successfully!"; | |
| } | |
| }); | |
| document.getElementById("myForm").addEventListener("submit", function(event) { | |
| event.preventDefault(); // Prevent submission to handle it with JS | |
| const name = document.getElementById("name").value.trim(); | |
| const email = document.getElementById("email").value.trim(); | |
| const message = document.getElementById("message"); | |
| if (name === "" || email === "") { | |
| message.textContent = "Please fill out all fields before submitting!"; | |
| } else { | |
| message.style.color = "green"; | |
| message.textContent = "Form submitted successfully!"; | |
| } | |
| }); |
| table.addEventListener("mouseout", (event) => { | ||
|
|
||
| if (event.target.tagName === "TD") { | ||
| for (let row of table.rows) { | ||
| for (let cell of row.cells) { | ||
| cell.classList.remove("highlight"); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
The mouseout event handler is inefficient and causes a noticeable flickering effect. It iterates over every cell in the table to remove the highlight class each time the mouse leaves a single cell. A much better approach is to use the mouseleave event on the table element itself. This event fires only once when the mouse pointer moves out of the entire table, allowing you to clear all highlights efficiently.
table.addEventListener("mouseleave", () => {
for (let row of table.rows) {
for (let cell of row.cells) {
cell.classList.remove("highlight");
}
}
});| }); | ||
|
|
||
| email.addEventListener("input", () => { | ||
| const emailPattern = /^[^ ]+@[^ ]+\.[a-z]{2,3}$/; |
There was a problem hiding this comment.
The email validation regex is too restrictive. It will reject many valid email addresses, such as those with subdomains (e.g., test@mail.co.uk), longer top-level domains (e.g., .international), or different character sets. Consider using a more robust regex to ensure valid emails are not rejected.
| const emailPattern = /^[^ ]+@[^ ]+\.[a-z]{2,3}$/; | |
| const emailPattern = /^\S+@\S+\.\S+$/; |
| form.addEventListener("submit", (e) => { | ||
| e.preventDefault(); | ||
|
|
||
| if ( | ||
| username.classList.contains("valid") && | ||
| email.classList.contains("valid") && | ||
| password.classList.contains("valid") | ||
| ) { | ||
| alert("Form submitted successfully!"); | ||
| } else { | ||
| alert("Please fix errors before submitting."); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The form submission logic relies on checking for a .valid class on each input. This approach is fragile. If a user uses browser autofill and clicks submit without interacting with the fields, the input events may not fire, the classes won't be added, and the form will be incorrectly considered invalid. A more robust solution is to explicitly run all validation checks for each field within the submit event handler itself before determining if the form is valid.
| editableDiv.addEventListener("click", () => { | ||
| const currentText = document.getElementById("textContent").textContent; | ||
| editableDiv.innerHTML = `<textarea>${currentText}</textarea>`; | ||
| }); |
There was a problem hiding this comment.
The click event listener is on editableDiv. When the user clicks to edit, the content is replaced by a <textarea>. However, a subsequent click inside the textarea will bubble up and re-trigger this same listener, causing the textarea to be re-rendered and losing any user input and focus. You should check the event.target to prevent this from happening when the textarea is already present.
editableDiv.addEventListener("click", (event) => {
if (event.target.tagName === 'TEXTAREA') {
return; // Already in edit mode, do nothing
}
const currentText = document.getElementById("textContent").textContent;
editableDiv.innerHTML = `<textarea>${currentText}</textarea>`;
});| level1.addEventListener("click", () => { | ||
| alert("Level 1 (Capture Phase)"); | ||
| }, true); | ||
|
|
||
| level2.addEventListener("click", () => { | ||
| alert("Level 2 (Bubble Phase)"); | ||
| }); | ||
|
|
||
| level3.addEventListener("click", () => { | ||
| alert("Level 3 (Target Phase)"); | ||
| }); |
There was a problem hiding this comment.
Using alert() for demonstration purposes is disruptive because it blocks the UI and halts script execution. For a better user and developer experience, it's recommended to use console.log(). This will output the event propagation sequence to the developer console without interrupting the application flow.
| level1.addEventListener("click", () => { | |
| alert("Level 1 (Capture Phase)"); | |
| }, true); | |
| level2.addEventListener("click", () => { | |
| alert("Level 2 (Bubble Phase)"); | |
| }); | |
| level3.addEventListener("click", () => { | |
| alert("Level 3 (Target Phase)"); | |
| }); | |
| level1.addEventListener("click", () => { | |
| console.log("Level 1 (Capture Phase)"); | |
| }, true); | |
| level2.addEventListener("click", () => { | |
| console.log("Level 2 (Bubble Phase)"); | |
| }); | |
| level3.addEventListener("click", () => { | |
| console.log("Level 3 (Target Phase)"); | |
| }); |
| @media (max-width: 768px) { | ||
| main { | ||
| flex-direction: column; | ||
| flex:1; |
There was a problem hiding this comment.
The flex: 1 property here has no effect because its parent element, <body>, does not have display: flex. This line is redundant and can be removed. Also, there should be a space between the property and value for consistency (flex: 1).
| flex:1; | |
| flex: 1; |
| parentDiv.addEventListener("click", function() { | ||
| alert("Parent div clicked!"); | ||
| }); | ||
|
|
||
| childDiv.addEventListener("click", function(event) { | ||
| alert("Child div clicked!"); | ||
| event.stopPropagation(); | ||
| }); |
There was a problem hiding this comment.
Using alert() is disruptive as it blocks the UI. For demonstrating event behavior, it's better to use console.log(), which logs messages to the developer console without interrupting the user or the application's execution flow.
| parentDiv.addEventListener("click", function() { | |
| alert("Parent div clicked!"); | |
| }); | |
| childDiv.addEventListener("click", function(event) { | |
| alert("Child div clicked!"); | |
| event.stopPropagation(); | |
| }); | |
| parentDiv.addEventListener("click", function() { | |
| console.log("Parent div clicked!"); | |
| }); | |
| childDiv.addEventListener("click", function(event) { | |
| console.log("Child div clicked!"); | |
| event.stopPropagation(); | |
| }); |
| if (mutation.addedNodes.length > 0) { | ||
| mutation.addedNodes.forEach((node) => { | ||
| if (node.tagName === "TEXTAREA") { | ||
| console.log("Textarea added to DOM ✅"); |
There was a problem hiding this comment.
This console.log statement appears to be for debugging. As per the best practices mentioned in the pull request description, such logs should be removed from the final code to keep the console clean in production.
| node.addEventListener("blur", () => { | ||
| const newText = node.value.trim() || "No text entered"; | ||
| editableDiv.innerHTML = `<span id="textContent">${newText}</span> <span class="icon">✏️</span>`; | ||
| }); |
There was a problem hiding this comment.
For a better user experience, the textarea should be automatically focused when it appears. This allows the user to start typing immediately without needing an extra click.
node.focus();
node.addEventListener("blur", () => {
const newText = node.value.trim() || "No text entered";
editableDiv.innerHTML = `<span id="textContent">${newText}</span> <span class="icon">✏️</span>`;
});1 participant
Terms and Conditions
HTML Best Practices
File Naming Convention:
Follow consistent and descriptive naming (e.g., dashboard.html, user-profile.html).
Use lowercase letters and hyphens instead of spaces.
Page Title:
Ensure the <title> tag is descriptive and aligns with the page content.
Include meaningful keywords for SEO if applicable.
Semantic Markup:
Use appropriate tags like <header>, <footer>, <section>, <article> for better readability and accessibility.
Accessibility Standards:
Ensure the use of alt attributes for images and proper labels for form elements.
Use ARIA roles where necessary.
Validation:
Ensure the code passes HTML validation tools without errors or warnings.
Structure and Indentation:
Maintain consistent indentation and proper nesting of tags.
Attributes:
Ensure all required attributes (e.g., src, href, type, etc.) are correctly used and not left empty.
CSS Best Practices
File Organization:
Use modular CSS files if applicable (e.g., base.css, layout.css, theme.css).
Avoid inline styles unless absolutely necessary.
Naming Conventions:
Use meaningful class names following BEM or other conventions (e.g., block__element--modifier).
Code Reusability:
Avoid duplicate code; use classes or mixins for shared styles.
Responsive Design:
Ensure proper usage of media queries for mobile, tablet, and desktop views.
Performance Optimization:
Minimize the use of unnecessary CSS selectors.
Avoid overly specific selectors and ensure selectors are not overly deep (e.g., avoid #id .class1 .class2 p).
Consistency:
Follow consistent spacing, indentation, and use of units (rem/em vs. px).
Maintain a single coding style (e.g., always use double or single quotes consistently).
Javascript Best Practices
File Organization:
Ensure scripts are modular and logically separated into files if needed.
Avoid mixing inline JavaScript with HTML.
Logic Optimization:
Check for redundancy and ensure the code is optimized for performance.
Avoid unnecessary API calls or DOM manipulations.
Solution Approach:
Confirm that the code solves the given problem efficiently.
Consider scalability for future enhancements.
Readability:
Use clear variable and function names.
Add comments for complex logic or algorithms.
Error Handling:
Ensure proper error handling for API calls or user input validation.
Code Quality:
Check for potential bugs (e.g., missing await, mishandling of null/undefined values).
Avoid unnecessary console.log statements in production code.
Security:
Avoid hardcoding sensitive data.
Sanitize user input to prevent XSS and other vulnerabilities.
Best Practices:
Use const and let instead of var.
Follow ES6+ standards where applicable.